Include EventId in telemetry properties#570
Conversation
…ta should be included in telemetry or not
|
@duracellko Thanks for your contribution! |
|
@duracellko Thanks for your patience! (I assure nobody will have to wait months for a PR review going forward) |
cijothomas
left a comment
cijothomas
left a comment
There was a problem hiding this comment.
Couple of changes requested.
I'd prefer to see the same approach used in the other PR I have mentioned about passing ApplicationInsightsLoggerOptions to both ApplicationInsightsLogger and Provider.
CHANGELOG.md
Outdated
| - Live metrics collection is enabled by default for .NET Core applications (was already enabled for full .NET applications). | ||
| - Updated Web/Base SDK version dependency to 2.5.0-beta1. | ||
| - DependencyCollector referred from 2.5.0-beta1 supports collecting SQL dependency calls in .NET Core Applications using EntityFramework. | ||
| - ApplicationInsightsLogger adds EventId into telemetry properties. It is off by default for compatibility. It can be switched on by configuring ApplicationInsightsLoggerOptions. |
There was a problem hiding this comment.
Can you move this to 2.3.0-beta1 please?
| public ApplicationInsightsLoggerProvider(TelemetryClient telemetryClient, IOptions<ApplicationInsightsLoggerOptions> options) | ||
| { | ||
| this.telemetryClient = telemetryClient; | ||
| this.filter = trueFilter; |
There was a problem hiding this comment.
Why use a true filter here?
There was a problem hiding this comment.
I followed pattern in ConsoleWriter, but it is not needed with new approach.
| /// <summary> | ||
| /// Options for configuration of <see cref="ApplicationInsightsLogger"/>. | ||
| /// </summary> | ||
| public class ApplicationInsightsLoggerOptions |
There was a problem hiding this comment.
There is another PR which introduces ApplicationInsightsLoggerOptions
https://github.com/Microsoft/ApplicationInsights-aspnetcore/pull/574/files
Can you also have took at that as well?
I think passing ApplicationInsightsLoggerOptions to both ApplicationInsightsLoggerProvider and ApplicationInsightsLogger as done in the other PR is a better option. Can you incorporate this change to your PR?
There was a problem hiding this comment.
Thank you for the hint. Yes, it's actually better. Also I realized that classes are internal, so I can change signature of constructors.
| Assert.True(isCorrectVersion); | ||
| } | ||
|
|
||
| /// <summary> |
There was a problem hiding this comment.
Thanks a lot for adding sufficient unit tests!
|
Thank you for comments. I will have a look at those later this week. |
2 participants
Fix issue #569.
Main update is to
ApplicationInsightsLogger. It includes EventId and EventName properties in telemetry. It is included only, when EventId has non-default values.By default this functionality is off for compatibility. Following example can switch it on:
This code should be ASP.NET Core application Startup class.
Additionally test coverage of ApplicationInsightsLogger was improved.